Pull request overview

This PR updates the Microsoft.Extensions.Hosting functional-test deployment infrastructure to make shutdown-related tests more reliable on constrained/Helix environments by hardening process launch and shutdown handling.

Changes:

• Add a retry loop when launching the self-hosted test process, and surface early-exit failures more consistently.
• Resolve the dotnet muxer path relative to the currently running shared framework (with PATH fallback) for Helix environments that don’t provide a global dotnet.
• Avoid Process.HasExited throwing during shutdown/cleanup when the Process object was never successfully started.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Holistic Review

Motivation: Justified. Linked issue #129832 shows a real, reproducible failure: ShutDownIfAnyHostProcess calls Process.HasExited on a Process that was created but never successfully started, throwing InvalidOperationException ("No process is associated with this object") during Dispose, which masks the real launch failure. The stack trace in the issue matches the code paths touched here.

Approach: Reasonable and targeted for test infrastructure. The IsRunning helper that swallows InvalidOperationException directly addresses the throwing HasExited call; adding throw; in the launch catch surfaces the real start failure instead of hitting the misleading HasExited/ExitCode access later; RunContinuationsAsynchronously avoids inline continuations on the Exited event thread; and GetHostDotNetExecutable resolves the muxer from the running host/shared-framework layout for Helix where dotnet isn't on PATH. These are consistent with the codebase and low-risk.

Summary: ⚠️ Needs Human Review. The code changes are individually sound and well-scoped test-infra hardening that plausibly fixes #129832, but this is a functional/Helix-only failure that cannot be reproduced or run in this environment, so a maintainer should confirm the fix against the actual crossgen2/nativeaot outerloop pipelines (as already requested in the PR thread). One documentation discrepancy should be corrected, and a couple of low-confidence robustness points are worth a glance.

Detailed Findings

✅ Correctness — Core fix is correct

IsRunning (ApplicationDeployer.cs) correctly treats a never-started/disposed Process as not running by catching InvalidOperationException, which is exactly the exception from the issue's stack trace. Replacing both !hostProcess.HasExited checks in ShutDownIfAnyHostProcess with IsRunning makes cleanup non-throwing without masking real state. Adding throw; in StartSelfHostAsync's catch is the right complement: it propagates the genuine launch error rather than letting the subsequent HostProcess.HasExited access throw a misleading InvalidOperationException.

⚠️ Documentation — PR description claims retry logic that is not in the diff

The PR description states: "Add retry logic for self-host process launch (3 attempts with short delay) to tolerate transient CI/Helix launch failures." The current base-to-head diff contains no retry loop — StartAndCaptureOutAndErrToLogger is still called exactly once inside a single try/catch that now rethrows. Please update the description (it notes it was AI-generated) so it matches the actual change, or add the retry if it was intended. As written, the description overstates the change.

💡 Robustness — GetHostDotNetExecutable fallback assumptions (low confidence, test-only)

Two minor, non-blocking observations on the new helper:

• The muxer fallback assumes dotnet lives exactly three directories above typeof(object).Assembly.Location (shared/Microsoft.NETCore.App/<version> → root). This holds for the standard testhost/dotnet layout and self-contained builds may differ, but the method falls back to DotnetCommandName, so worst case is the prior behavior. Fine as-is.
• Process.GetCurrentProcess().MainModule?.FileName can throw on some restricted platforms; in the normal test host it is fine. Given this is test infrastructure and the failure mode is only a fallback, not worth guarding, but flagging for visibility.

✅ Concurrency — RunContinuationsAsynchronously and reordering

Switching started to TaskCreationOptions.RunContinuationsAsynchronously avoids running the awaiting continuation inline on the process Exited/output-reader thread, which is a good default. Moving the hostExitTokenSource declaration earlier is a no-op reorder with no semantic impact.

Generated by Holistic Review · 59 AIC · ⌖ 10.9 AIC · ⊞ 10K

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

                    // Surface the real launch failure instead of letting it be masked later during disposal.
                    Logger.LogError("Error occurred while starting the process. Exception: {exception}", ex.ToString());
                    throw;

        // The deployer launches the test app through the dotnet muxer. The NativeAOT and ReadyToRun
        // test legs publish the test itself as a self-contained app, so there is no muxer sitting next
        // to the running host for Process.Start to resolve. The child app would run on the ordinary
        // shared framework anyway, so those legs gain nothing from these tests; the single file test
        // runner turns RemoteExecutor off for the same reason.
        public static bool IsPortableAppLaunchSupported => PlatformDetection.IsNotNativeAot && !PlatformDetection.IsReadyToRunCompiled;

                try
                {
                    HostProcess.StartAndCaptureOutAndErrToLogger(executableName, Logger);
                }

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

            else
            {
                Logger.LogWarning("Host process already exited or never started successfully.");
            }

                catch (Exception ex)
                {
                    // Surface the real launch failure instead of letting it be masked later during disposal.
                    Logger.LogError("Error occurred while starting the process. Exception: {exception}", ex.ToString());
                    throw;

                try
                {
                    HostProcess.StartAndCaptureOutAndErrToLogger(executableName, Logger);
                }

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

            else
            {
                Logger.LogWarning("Host process already exited or never started successfully.");
            }

        protected void ShutDownIfAnyHostProcess(Process hostProcess)
        {
            if (hostProcess is not null && IsRunning(hostProcess))
            {
                Logger.LogInformation("Attempting to cancel process {0}", hostProcess.Id);

                catch (Exception ex)
                {
                    // Surface the real launch failure instead of letting it be masked later during disposal.
                    Logger.LogError("Error occurred while starting the process. Exception: {exception}", ex.ToString());
                    throw;